Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

disable ACE scaled alerts per radiation working group - #2

Open
bspitzbart wants to merge 2 commits into
masterfrom
disable_scaled
Open

disable ACE scaled alerts per radiation working group#2
bspitzbart wants to merge 2 commits into
masterfrom
disable_scaled

Conversation

@bspitzbart

Copy link
Copy Markdown

No description provided.

@swolk

swolk commented Jun 6, 2013

Copy link
Copy Markdown

do I need to do this action, or can you, once Tom is happy?

It looks fine to me.

The other change, the 1 hour wait on SCS 107 re-arm, is trickier

On Thu, Jun 6, 2013 at 10:30 AM, bspitzbart notifications@github.comwrote:


You can merge this Pull Request by running

git pull https://github.com/mta/space_weather disable_scaled

Or view, comment on, or merge it at:

#2
Commit Summary

  • disable ACE scaled alerts per radiation working group

File Changes

Patch Links:

*-Scott *


swolk@cfa.harvard.edu

Chandra X-Ray Center
Harvard-Smithsonian
Center for Astrophysics | 617-496-7766
60 Garden St. MS-70 | 617-721-4360 (cell phone)
Cambridge, MA 02138 | 617-495-7356 (fax)
http://hea-www.harvard.edu/~swolk

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented out line is different from the original, which is confusing.

In general, now that you are using a version control system that makes it easy to track not only the code changes but also provide detailed commentary on the how and why, it's better to not leave old lines of code as comments. The github-workflow way to do this is to put a summary of the radiation working group decision into the pull request discussion (complete with links to the twiki page), along with a short (could be one or two line) summary of what was done in the code to effect the change.

Comment thread G13_red_viol.pl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bspitzbart - it looks like you started adding an entirely different patch, which is the SCS107 lockout. It's really much better if you do one functional change per pull request, particularly since the original was simple and didn't really require testing (vs. this update which will need to be tested). Maybe including this update was accidental?

@taldcroft

Copy link
Copy Markdown
Member

@bspitzbart - at this point I would suggest that you essentially start over by going back to master and making a new branch with a somewhat different name. The required code patch is really simple, just delete the two code blocks that currently test the scaled p5 and p6 rates. Take out the entire code blocks and leave only emptiness, but put in a good commit message, that might look something like:

Remove code that issues scaled p5 and p6 alerts

Two code blocks within process_ace.nawk were responsible for testing the
scaled p5 -> p3 and p6 -> p3 rates.  If these exceeded the thresholds then
another script was called to actually send the alerts.  Based on a decision
by the radiation working group these scaled alerts are no longer desired.

Note the very short (< 70 characters) commit title, followed by a blank line, followed by a detailed description. The detailed description isn't always required, but in this case would be quite useful. Following this format is useful for github because the messages show up nicely formatted.

That's all that will be needed and this can go through.

@taldcroft

Copy link
Copy Markdown
Member

BTW, when you format the commit as above you should see the magic when you issue the pull request, which is that it is all filled in nicely.

@taldcroft taldcroft mentioned this pull request Aug 15, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants